Skip to content

[dotnet] [bidi] Get tree command returns GetTreeResult object #15978

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jun 29, 2025
Merged

[dotnet] [bidi] Get tree command returns GetTreeResult object #15978

merged 3 commits into from
Jun 29, 2025

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jun 29, 2025 •
edited by qodo-merge-pro bot
Loading

User description

💥 What does this PR do?

Be more closer to low-level specification.

🔧 Implementation Notes

User is still able to use previous code, like

var tree = await bidi.BrowsingContext.GetTreeAsync();
var theFirst = tree[0];
// or
var theFirst = tree.First();

And additionally:

var tree = await bidi.BrowsingContext.GetTreeAsync();
var allContext = tree.Contexts;

In general we reveal nested Enumerable property of Enumerable Result to support more properties in future.

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement

Description

  • Refactor BiDi result objects to expose nested properties

  • Make GetTreeResult implement IReadOnlyList<BrowsingContextInfo>

  • Add Contexts property to GetTreeResult for better API access

  • Update multiple result classes to use public properties instead of private fields

Changes diagram

flowchart LR
  A["Private Fields"] --> B["Public Properties"]
  B --> C["IReadOnlyList Implementation"]
  C --> D["Enhanced API Access"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
8 files
GetTreeCommand.cs
Refactor GetTreeResult to implement IReadOnlyList               
+18/-1   
BrowsingContextModule.cs
Update GetTreeAsync return type                                                   
+2/-5     
BrowsingContext.cs
Update GetTreeAsync method signature                                         
+1/-2     
GetTreeResultConverter.cs
Add JSON converter for GetTreeResult                                         
+43/-0   
GetClientWindowsCommand.cs
Expose ClientWindows as public property                                   
+9/-7     
GetUserContextsCommand.cs
Expose UserContexts as public property                                     
+7/-7     
LocateNodesCommand.cs
Expose Nodes as public property                                                   
+7/-7     
GetCookiesCommand.cs
Expose Cookies as public property                                               
+7/-7     
Configuration changes
1 files
Broker.cs
Register GetTreeResultConverter                                                   
+1/-0     
Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @selenium-ci selenium-ci added the C-dotnet .NET Bindings label Jun 29, 2025
    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Inconsistent Pattern

    The Nodes field is declared as readonly instead of a property like other similar classes in this PR. This breaks consistency with the established pattern of using properties.

     
    public readonly IReadOnlyList<Script.NodeRemoteValue> Nodes;
    Missing Import

    The System.Collections import is added but IEnumerable cast suggests it might be unnecessary. Verify if this import is actually needed or if explicit casting can be avoided.

     
    using System.Collections;
using System.Collections.Generic;
    Incomplete Implementation

    The Write method throws NotImplementedException which could cause runtime errors if serialization is attempted. Consider implementing or documenting why serialization is not supported.

     
    public override void Write(Utf8JsonWriter writer, GetTreeResult value, JsonSerializerOptions options)
{
    throw new NotImplementedException();
}

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 29, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    ✅ Convert field to property
    Suggestion Impact:The suggestion was directly implemented - the public readonly field was converted to a property with a getter as suggested

    code diff:

    -    public readonly IReadOnlyList<Script.NodeRemoteValue> Nodes;
+    public IReadOnlyList<Script.NodeRemoteValue> Nodes { get; }

    The Nodes field should be a property with a getter to maintain consistency with
    other similar classes in the codebase. This ensures proper encapsulation and
    follows C# conventions.

    dotnet/src/webdriver/BiDi/BrowsingContext/LocateNodesCommand.cs [47]

    -public readonly IReadOnlyList<Script.NodeRemoteValue> Nodes;
+public IReadOnlyList<Script.NodeRemoteValue> Nodes { get; }

    [Suggestion processed]

    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly points out that Nodes is a public field, while other similar classes in the PR use properties, and changing it improves consistency.

    		Low
    Learned
    best practice
    Add null validation before usage

    The code uses null-forgiving operator (!) without proper validation, which can
    lead to runtime exceptions. Add proper null checking before creating the
    GetTreeResult instance to ensure the deserialized contexts is not null.

    dotnet/src/webdriver/BiDi/Communication/Json/Converters/Enumerable/GetTreeResultConverter.cs [31-37]

     public override GetTreeResult Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
 {
     using var doc = JsonDocument.ParseValue(ref reader);
     var contexts = doc.RootElement.GetProperty("contexts").Deserialize(options.GetTypeInfo<IReadOnlyList<BrowsingContextInfo>>());
 
-    return new GetTreeResult(contexts!);
+    ArgumentNullException.ThrowIfNull(contexts);
+    return new GetTreeResult(contexts);
 }
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why:
    Relevant best practice - Add null checks and validation for parameters, properties, and return values before using them to prevent NullReferenceException and other runtime errors. This includes checking dictionary keys exist before accessing them, validating required JSON properties, and ensuring objects are not null before method calls.

    		Low
    • Update

    @nvborisenko
    Copy link
    Member Author

    Thanks Nick

    @nvborisenko nvborisenko merged commit dda35b2 into SeleniumHQ:trunk Jun 29, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the get-tree-result branch June 29, 2025 21:34
    This was referenced Sep 12, 2025
    Closed
    Closed
    Merged
    This was referenced Sep 19, 2025
    Closed
    Closed
    Closed
    This was referenced Oct 1, 2025
    Closed
    Closed
    Closed
    Merged
    Closed
    Closed
    Closed
    This was referenced Oct 20, 2025
    Closed
    Closed
    Open
    Open
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    C-dotnet .NET Bindings Review effort 3/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @nvborisenko @selenium-ci